-
Notifications
You must be signed in to change notification settings - Fork 201
feat(contextual-help): S2 migration #3909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: spectrum-two
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 662f926 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🚀 Deployed on https://pr-3909--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.42 MB*
contextualhelp
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
3bdff09
to
1850227
Compare
de08037
to
57323f8
Compare
c56c432
to
f237622
Compare
f237622
to
40e2b6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looking great. Well documented and thanks for the storybook clean-up. I left one small suggestion about how we document high contrast hooks.
1bceda1
to
75a08f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions for you!
-
Is there a reason we have
customStyles
on every story? Can we get rid of any and use our CSS instead? I haven't actually tried removing them, so we might, I'm not sure. -
I couldn't comment on the action button code, but I think we need to gather the
globals
data so that we can appropriately render both the small action button for large/mobile, and the XS action button for medium/desktop
// before the return statement
const { globals = {} } = context;
const scale = globals.scale ?? "medium"; // this const may not be necessary, so you could refactor this out if you wanted
// then in the ActionButton call, change the size declaration:
size: scale === "medium" ? "xs" : "s",
- Could you double check all of the mods in the CSS? There were quite a few places where I was seeing
--mod-spectrum-contextual-help*
instead of just--mod-contextual-help*
. (places where it was already like that, I mean) - How does contextual help get triggered? Would we want to add
isSelected: false
to the action button at all, just to match the design, or is this component triggered because an action button was selected?
@@ -180,74 +190,14 @@ TopPopover.storyName = "Default - top popover"; | |||
export const HelpDefault = Template.bind({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we removed the other help icon stories, what do you think about just adding some extra documentation here about them?
"All variants and positions of the contextual help component can be configured with a help icon. When display help or resources to learn more..." and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added:
All variants and positions of the contextual help component can be configured with a help icon.
I think that's clear enough inasmuch as it clarifies how it's used and what sets it apart from other use cases. We can add more, but the icon alone distinguishing it makes me hesitant to do so. ✨
--spectrum-contextual-help-body-line-height: var(--spectrum-line-height-200); | ||
|
||
--spectrum-contextual-help-body-color: var(--spectrum-body-color); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to set the --mod-popover-corner-radius
passthrough at all? That would mean the mod and the --spectrum
custom props are the same value, but if they ever change to be different, it could be nice to already have that passthrough set up for contextual help.
I also think we've gotten feedback that we need to be more explicit with the cascades, so in my head, that means setting the mod passthroughs even if they're the same value for now. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question for --mod-popover-animation-distance
. That mod for the distance between the popover and its source specifically gave us some trouble in picker initially, so my gut says just define it for contextual help (even though again, I think it's technically the same as the --spectrum
variable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me — we set --mod-popover-corner-radius
in the coach mark migration and that leveraged the popover extensively. I went ahead and dropped a passthrough section in here as well. ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took one more look at this and could definitely use some eyes on it to see if I threaded it through everything properly. 😅
aca04a3
to
cb8314b
Compare
cb8314b
to
ef4e9c5
Compare
6bff760
to
662f926
Compare
Description
CSS-819
S2 migration for contextual help
This migrates the
contextual help
component to the latest Spectrum 2 designs. Custom properties have been remapped and added per the design specification.Typographic and color tokens have been updated per design specifications.
All existing popover positioning varations are supported.
New custom properties
--spectrum-contextual-help-body-color
--spectrum-contextual-help-body-line-height
--spectrum-contextual-help-body-sans-serif-font-family
--spectrum-contextual-help-body-sans-serif-font-style
--spectrum-contextual-help-body-sans-serif-font-weight
--spectrum-contextual-help-title-color
--spectrum-contextual-help-title-font-style
--spectrum-contextual-help-title-font-weight
--spectrum-contextual-help-title-line-height
--spectrum-contextual-help-title-sans-serif-font-family
New mods
--mod-contextual-help-body-line-height
--mod-contextual-help-body-sans-serif-font-family
--mod-contextual-help-body-sans-serif-font-style
--mod-contextual-help-body-sans-serif-font-weight
--mod-contextual-help-body-size
--mod-contextual-help-title-color
--mod-contextual-help-title-font-style
--mod-contextual-help-title-font-weight
--mod-contextual-help-title-line-height
--mod-contextual-help-title-sans-serif-font-family
--highcontrast-contextual-help-heading-color
--highcontrast-contextual-help-title-color
Removed mods
--highcontrast-contextual-help-heading-color
Screenshots
To-do list